Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send batch events for DiagnosticsUpdatedArgs #70039

Merged
merged 6 commits into from
Oct 12, 2023
Merged

Send batch events for DiagnosticsUpdatedArgs #70039

merged 6 commits into from
Oct 12, 2023

Conversation

sharwell
Copy link
Member

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 20, 2023
@CyrusNajmabadi
Copy link
Member

FWIW, with the intent being to entirely remove this ysstem, my preference would be to hold off on larger design changes here.

@sharwell sharwell force-pushed the batch-updates branch 2 times, most recently from 0a0fdff to 65c10de Compare September 20, 2023 21:14
This was referenced Sep 21, 2023
Open
Closed
@sharwell
Copy link
Member Author

@CyrusNajmabadi This change affects both IDiagnosticUpdateSource.DiagnosticsUpdated and IDiagnosticService.DiagnosticsUpdated. It is a very high impact change to improve the experience for anyone with either of these services enabled. Are there immediate plans to fully remove both of these?

@sharwell sharwell marked this pull request as ready for review September 29, 2023 13:45
@sharwell sharwell requested a review from a team as a code owner September 29, 2023 13:45
@ToddGrun
Copy link
Contributor

ToddGrun commented Oct 3, 2023

I've looked through this just a bit and it looks really nice. Do you have any idea on roughly how many diagnostic events (task creations) this prevents? @CyrusNajmabadi -- Is this worth pursuing, or are both of these services slated for removal soon?

@sharwell
Copy link
Member Author

sharwell commented Oct 3, 2023

Do you have any idea on roughly how many diagnostic events (task creations) this prevents

We've seen some traces that suggest thousands or tens of thousands over a short period of time for some user scenarios.

ToddGrun
ToddGrun reviewed Oct 3, 2023
View reviewed changes
.../Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs Outdated

if (argsBuilder.Count > 0)
{
AnalyzerService.RaiseDiagnosticsUpdated(argsBuilder.ToImmutableAndClear());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AnalyzerService.RaiseDiagnosticsUpdated(argsBuilder.ToImmutableAndClear());

Wanting to make sure this needs to be done inside the loop

Copy link
Member Author

@sharwell sharwell Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ Yes, we don't want to delay the presentation of diagnostics from analyzer A until some other analyzer B completes. We only batch events in cases where multiple events are ready at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the explanation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a dumb question, but would it make sense to try to utilize AsyncBatchingWorkQueue to allow these to get batched together if they do completely quickly? Another dumb question is why these are awaited serially, and not kicked off in a parallel fashion? @mavasani?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a dumb question, but would it make sense to try to utilize AsyncBatchingWorkQueue to allow these to get batched together if they do completely quickly?

Potentially, yes. In DefaultDiagnosticAnalyzerService.cs I added a comment to that effect.

Another dumb question is why these are awaited serially, and not kicked off in a parallel fashion?

I'm not sure, but it's outside the scope of this change.

Copy link
Contributor

@mavasani mavasani Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another dumb question is why these are awaited serially, and not kicked off in a parallel fashion?

There is some history here. Before we moved core analyzer execution pieces to OOP, we wanted to make sure that we don't stress the devenv process by running analyzers concurrently in proc as this was part of continuous background analysis and led to high CPU usage, GC and UI delays. After moving the analyzers to OOP, we do execute all the analyzers in OOP process in parallel. Just the requests to fetch the analysis result still happens serially here, which can potentially be changed but will not likely lead to any significant performance benefit. Note that we do special case the compiler analyzer though and execute it in isolation upfront, rest of the analyzers execute concurrently. For example, see below:

roslyn/src/Features/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor.cs

Lines 216 to 249 in 49e5f55

private async Task<ImmutableArray<DiagnosticData>> GetSyntaxDiagnosticsAsync(DiagnosticAnalyzer analyzer, bool isCompilerAnalyzer, CancellationToken cancellationToken)
{
// PERF:
// 1. Compute diagnostics for all analyzers with a single invocation into CompilationWithAnalyzers.
// This is critical for better analyzer execution performance.
// 2. Ensure that the compiler analyzer is treated specially and does not block on diagnostic computation
// for rest of the analyzers. This is needed to ensure faster refresh for compiler diagnostics while typing.
RoslynDebug.Assert(_compilationWithAnalyzers != null);
RoslynDebug.Assert(_compilationBasedAnalyzersInAnalysisScope.Contains(analyzer));
if (isCompilerAnalyzer)
{
if (AnalysisScope.TextDocument is not Document)
{
return ImmutableArray<DiagnosticData>.Empty;
}
return await GetCompilerAnalyzerDiagnosticsAsync(analyzer, AnalysisScope.Span, cancellationToken).ConfigureAwait(false);
}
if (_lazySyntaxDiagnostics == null)
{
using var _ = TelemetryLogging.LogBlockTimeAggregated(FunctionId.RequestDiagnostics_Summary, $"{nameof(GetSyntaxDiagnosticsAsync)}.{nameof(GetAnalysisResultAsync)}");
var analysisScope = AnalysisScope.WithAnalyzers(_compilationBasedAnalyzersInAnalysisScope);
var syntaxDiagnostics = await GetAnalysisResultAsync(analysisScope, cancellationToken).ConfigureAwait(false);
Interlocked.CompareExchange(ref _lazySyntaxDiagnostics, syntaxDiagnostics, null);
}
return _lazySyntaxDiagnostics.TryGetValue(analyzer, out var diagnosticAnalysisResult)
? diagnosticAnalysisResult.GetDocumentDiagnostics(AnalysisScope.TextDocument.Id, AnalysisScope.Kind)
: ImmutableArray<DiagnosticData>.Empty;
}

@CyrusNajmabadi
Copy link
Member

Tough call here. I totally see the desire to resolve the problem with insanely bad task-chains and whatnot. But this is also fairly complex in an area where complexity has already been a challenge.

Overall it does look ok to me though. But i'd like at least one more pass from @mavasani to ensure this will be ok. If everyone feels reasonable about this, i'm ok moving forward.

mavasani
mavasani reviewed Oct 4, 2023
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @sharwell. Overall, this looks very promising from performance perspective. I am definitely concerned if this degrades the user experience though. As we delay push notifications after all analyzers have finished executing, all error list refresh operations will be delayed. More specifically, my concern is that the compiler analyzer is generally one of the first ones to execute and we send out push notifications for it first to ensure compiler diagnostics are refreshed as soon as possible and not affected by slower analyzers - we have separate code paths in DocumentAnalysisExecutor to ensure that compiler analyzer is executed upfront as a single analyzer, whose execution and reporting does not get clubbed with other analyzers to help with faster refresh. We definitely need to ensure that this behavior is retained even after your change - maybe treat compiler analyzer specially and raise push requests for it as soon as they are available and don't add it to the batch. That likely makes the code lose readability, but I think we can't assess that unless you have addressed it (maybe it's not as widespread as I think).

Another concern here is that we plan to turn on LSP pull diagnostics by default soon enough, and this change will not get any dogfooding, unless someone explicitly turns off pull diagnostics. Any regression from this change might not get caught until after a release if and when customers try to switch back to non-LSP pull diagnostics mode.

I think both of these concerns are addressable though, especially if we feel this gives a large performance improvement.

@ToddGrun
Copy link
Contributor

ToddGrun commented Oct 4, 2023

LGTM, I'll let Cyrus or Manish do the actual signoff though

mavasani
mavasani approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! This should definitely bring a good performance improvement - please do make sure to add some more comments and also locally verify the editing experience on RoslynDevHive to make sure we did not regress the user experience here. Thanks @sharwell!

ToddGrun added a commit to ToddGrun/roslyn that referenced this pull request Oct 7, 2023
@ToddGrun
Naive approach to simulate what is being done in dotnet#70039
c150c6c
@sharwell sharwell enabled auto-merge October 9, 2023 13:49
@sharwell sharwell disabled auto-merge October 12, 2023 18:42
@sharwell sharwell merged commit 6c6fa1e into dotnet:main Oct 12, 2023
24 checks passed
@sharwell sharwell deleted the batch-updates branch October 12, 2023 18:42
@ghost ghost added this to the Next milestone Oct 12, 2023
@allisonchou allisonchou mentioned this pull request Oct 13, 2023
Closed
@jjonescz jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023
sharwell added a commit to sharwell/roslyn that referenced this pull request Feb 3, 2024
@sharwell
Fix incorrect assumption in test
c5df7b6 
Corrects a mistake introduced in dotnet#70039 but not revealed by tests due to
the way exceptions were caught.
@sharwell sharwell mentioned this pull request Mar 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@mavasani mavasani mavasani approved these changes

@ToddGrun ToddGrun ToddGrun approved these changes

Assignees
No one assigned
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.9 P1
Development

Successfully merging this pull request may close these issues.
5 participants
@sharwell @CyrusNajmabadi @ToddGrun @mavasani @jjonescz